-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
module: Private internal exports subpaths #33780
Conversation
//cc @nodejs/modules-active-members |
We still haven't discussed this in the modules group; it continues to feel rushed when the work is done on a PR before the semantics have been discussed. Perhaps this could be marked as a draft until that happens? |
The PR is only 58 lines added, 10 removed. That's almost trivial. I think we can review both the code and the design here; on the design side it's also not that big of a change. |
Could we get a test around import of |
Giving my 2 cents here for inspiration on the discussion: Even while there seem like no technical issues with using Regarding dx ergonomics I dislike that the developer has to repeat the package name everytime they use a private reference. This makes the source code package name dependent. It feels a bit off to me that private things also use the As alternative I propose using an
|
Specifiers are URLs, so that is a problem! |
Yes and no. Relative and absolute specifiers are URLs, but “bare” specifiers ( |
Correct. Specifiers are generally not URLs. They are specifiers. Specifiers that start with specific characters (e.g. “./“) or can be parsed as valid URL strings will resolve “trivially” to URLs. But the specifiers themselves aren’t URLs. E.g. bare specifiers are allowed to have characters that wouldn’t be valid in URLs. They just need to go away during resolution to a module URL. |
I thought we established that specifiers can be URLs towards the bottom of #49448.
Additionally, when web browsers resolve a module specifier, the first thing they do is …
Refs: https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier Something perhaps worth considering in regards to the application of the URL parser: will it cause complications when resolving module specifiers w/ URL fragments (i.e., interpret the specifier as an invalid URL and thus result in failure)? |
“exports” is about public exports; it doesn’t make sense to me to put internal/private things in there (and doing so seems like it will dramatically complicate resolution tooling) Why wouldn’t we want another field for this, one self-reference also respects? |
I maybe should've highlighted that part more but your quote from the HTML spec is what I was referring to with the bolded bit here:
Bare specifiers (which is the only position where
At least as spec'd, there's no such thing as a bare specifier with URL fragments (or query arguments). |
I think that would be possible. The downsides are:
I don't have a strong opinion either way but I think the approach in this PR ("maximally incremental") is viable and has one of the lowest possible teaching overheads - which I like. "If you want the |
What about someone trying to migrate seamlessly from "exports", but they have a file that starts with |
Yes, that would break. But I think giving them the choice between renaming the file to not start with |
Do "exports" explicitly support fragments on the left hand side during resolution or is that a bit of undefined behavior? In the docs it doesn't seem clear and it would be surprising if it picked up fragments given that require('./#foo'); // resolves in "." -> path "./#foo"
import('./#foo'); // resolves in "." -> URL pathname "./" hash fragment "#foo"
import(encodeURIComponent('./#foo')); // resolves in "." -> URL pathname "./#foo" With this PR, the semantics of require('pkg/#foo'); // resolves in pkg, but # segment now is something new (internal export fragment?), not a hash fragment
import('pkg/#foo'); // resolves in pkg, but # segment now is something new, not a hash fragment
import(encodeURIComponent('pkg/#foo')); // this still gets the file starting with #? I'm a bit concerned about the overlap here being a bit unclear. |
In webpack We didn't implement fragments at all, but I would expect them to behave similar to query string. So I think it a little bit weird to use |
I created this gist to illustrate the expected behavior for URL-style specifiers when using When using
We could pick a different character but I'm not sure there's one that expresses "this is private" more clearly than |
I'd also lean on wanting to avoid making the syntax more complex with characters having multiple meanings. I think supporting this feature might also not be worth its weight if the complexity in explaining how exactly it overlaps is too high. Maybe we could get a better understanding of if we could make an invalid URL or overlap that is highly defined like when import maps tried to use |
The "imports" field always was the original design for this use case, which grew out of an original proposal posted in March 2019 to the exports spec repo at jkrems/proposal-pkg-exports#30, and finally leading to jkrems/proposal-pkg-exports#40 in August 2019. The concern we have with the There is certainly middle ground to be explored here though and I could get behind the My concern was just if it would be realistically possible to ship a whole new |
The problem we are dealing with here is that tools like Webpack are now supporting both internal rewriting of modules with the For this reason I think it is important that we move forward on a solution here soon, but I can't really tell where consensus might lie at this point since. I suggested this PR as it is simple and straightforward and gets us there. But I do not know how to progress with the objections further - if I work on a PR to support Can the objectors please suggest a path forward here? |
My personal objection/concern is largely around conflating the "exports" field with internal-only usage; for me a separate field would be the only path forward. The "browser" field has always supported internal rewriting of modules, so I'm not sure why it's surprising that the "browser" condition is being used that way. |
I think it depends on how abstractly But I don't have strong opinions on |
The problem is that this means the browser field provides useful features the exports field does not - there the exports field is simply not feature-complete, and cannot be considered a replacement for the browser field.
Can I ask then explicitly if anyone objects to an |
To reiterate since it seems there is some silence on stances. I'd heavily support "imports" rather than this. |
It's like an "imports map" that affects only this package. |
Could even be |
@ljharb for the initial implementation I would like to stick with exactly what was originally proposed in https://github.com/jkrems/proposal-pkg-exports/blob/master/README.md#3-imports-field. I do personally |
Closing as the imports field has been merged in #34117. |
This PR defines private exports subpaths to be those exports starting with
./#
, and then throws anERR_PRIVATE_PACKAGE_PATH
when attempting to resolve these exports from outside of the package, so that they can only be resolved via package self-resolution.A package that defines these mappings today, for example:
will already work correctly under the current exports implementation in Node.js 12 such that
pkg/#internal
can be imported and required from outside of the package and from within the package using package own name self-resolution.This PR only adds a new validation phase to these resolutions that throws the error when importing from outside of the package, thereby creating a new private encapsulation layer. This keeps the implementation simple and backwards compatible as opposed to defining a whole new type of private resolution.
Motivation
Package exports provide a number of features both to the external API of the package imported by consumers of the package (
import 'pkg/x'
) and also to the internal resolutions used by the package author themselves when using self-resolution (import 'pkg/x'
from within modules of the package itself).The issue is that any time a package author might want to take advantage of exports features using own-name self-resolution such as for internal directory mappings, avoiding the need for file extensions, or providing internal conditional resolutions, by using the
"exports"
field they must make these modules public by default.For these use cases, it can be useful to allow package authors to have private internal resolutions that only apply for internal self-resolution but aren't defined as part of the public exports interface of the package.
The
#
symbol has been chosen as its meaning matches that of private fields in classes.This effectively then also provides a solution to the browser field "internal file mappings" use case for the exports field, allowing packages to swap out internal modules based on environment conditions, thus fully replacing the functionality of the browser field for arbitrary condition handling in exports.
Prior art:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes